stop using Base.convert #66

visr · 2022-08-03T22:28:58Z

Following discussion in #49.

I tried implementing Base.convert as suggested by the GeoInterface docs for GeometryBasics, testing with GeoJSON geometries.

The first method is needed to send the 2-arg convert through the 3-arg method with the trait argument.

function Base.convert(::Type{T}, geom) where {T<:AbstractGeometry}
    return convert(T, GeoInterface.geomtrait(geom), geom)
end

This method is geom::Any, so if there are any convert methods with more specific types already defined, I cannot hit this anymore. This was the case with trying to convert(GeometryBasics.Point, p::GeoJSON.Point). GeoJSON.Point is an AbstractVector, and there is a method defined for those already. However that method doesn't work (since the number of dimensions, 2 or 3, is not statically known).

So here I removed all Base.convert usages from docs and examples. There is already a GeoInterface.convert method

convert(T, geom) = convert(T, geomtrait(geom), geom)

That works well, and I figured we don't need to ask developers to add a fast passthrough if we can define it ourselves in fallback.jl:

# no conversion needed
convert(::Type{T}, ::AbstractTrait, x::T) where {T} = x

visr · 2022-08-03T22:43:49Z

Probably it would be better to move the "no conversion needed" next to the 2 to 3 arg method, and make it 2 arg, avoiding the geomtrait call.

And expand the example a bit.

evetion · 2022-08-07T03:27:17Z

@rafaqz and I discussed this in the past and we settled for using Base.convert. While harder to implement, it enables more automatic conversions and it users know it already.

In terms of quick fallback, for LibGEOS, I did the following: JuliaGeo/LibGEOS.jl@b6902e1.

evetion · 2022-08-07T03:30:30Z

I haven't tested the GeometryBasics code yet, but in your example:

This method is geom::Any, so if there are any convert methods with more specific types already defined, I cannot hit this anymore. This was the case with trying to convert(GeometryBasics.Point, p::GeoJSON.Point). GeoJSON.Point is an AbstractVector, and there is a method defined for those already. However that method doesn't work (since the number of dimensions, 2 or 3, is not statically known).

The conversion would still work for any non GeoJSON.Point geometries right? Also, the GeoJSON conversion, if defined, could be removed in favour of the generic GeoInterface conversion?

visr · 2022-08-07T05:19:55Z

The conversion would still work for any non GeoJSON.Point geometries right?

No, it would only work for geometries for which Any is the most specific method it hits, which is something that cannot be relied on.

Also, the GeoJSON conversion, if defined, could be removed in favour of the generic GeoInterface conversion?

I'm talking about the generic GeoInterface conversion, which is the only one I implemented. I was testing using GeoJSON as source geometries.

visr · 2022-08-07T18:34:18Z

So does that mean that this function in this package:

convert(T, geom) = convert(T, geomtrait(geom), geom)

Accidentally defined GeoInterface.convert and was supposed to have been Base.convert(T, geom) = ...?

While harder to implement, it enables more automatic conversions and it users know it already.

For those reasons I would've also preferred Base.convert if it could work in general. But this interface is built around traits, and not around subtyping. So it would be strange if for this one function it would depend on subtyping instead. We want to be able to support geometry types like:

struct LineString <: AbstractVector{Float64}
    # [p1.x p1.y p2.x p2.y ...]
    coords::Vector{Float64}
end

right? For types like this it can be useful to make them an AbstractVector.

rafaqz · 2022-08-08T13:33:06Z

Yeah, its a bit annoying because of the manual definitions required.

But Base.convert is a bit of a special case. Its the generic way of converting things in Julia, and Base calls it in places, like building structs and assinging to arrays, function return types etc.

I think thats nice to have for all geometry types?

convert here only gives us manual conversion.

evetion · 2022-08-13T20:27:27Z

Accidentally defined GeoInterface.convert and was supposed to have been Base.convert(T, geom) = ...?

Sorta. I started out with convert and forgot to change all references to Base.convert.

jw3126 · 2022-11-19T12:45:16Z

I think it would be great to make a decision here. Personally, I am not in favor of overloading Base.convert, in addition to the reasons mentioned above such overloads can create ambiguities in my experience. One ecosystem creates a Base.convert(::Type{Something}, ::Any), another creates Base.convert(::Type{<:Any}, ::SomethingElse) and if you combine both you get ambiguities.
See for instance JuliaInterop/CxxWrap.jl#313 (comment) for a recent example.

rafaqz · 2022-11-28T09:41:12Z

Hmm I'm tending to agree with you ans @visr now, maybe GeoInterface.convert is better.

evetion · 2022-11-28T09:48:18Z

Agreed, and maybe also geoplot (like graphplot for graphs) should make things way easier as well.

rafaqz · 2022-11-28T09:59:50Z

Ok. well lets put in some time to get GeoInterface.convert working everywhere, then think about package level conversions.

I don't think we can delete the Base.convert methods here though, probably we have to leave them but switch documentation to GeoInterface.convert.

visr · 2022-12-30T20:08:47Z

I don't think we can delete the Base.convert methods here though, probably we have to leave them but switch documentation to GeoInterface.convert.

Why not? There is only one method add to Base.convert, which always throws an error:

Base.convert(T::Type, ::AbstractGeometryTrait, geom) = error("..")

If we remove it, calling this methods will still throw an error, just a MethodError instead of an ErrorException.

fingolfin · 2022-12-31T15:22:15Z

src/interface.jl

-have implemented GeoInterface.
+Create a `CustomGeom` from any `geom` that implements the GeoInterface.
 """
 convert(T, geom) = convert(T, geomtrait(geom), geom)


If this was changed to Base.convert, it would be highly problematic -- but not just because of potential ambiguities this introduces, but also because it would invalidate basically every method in the system, forcing Julia to recompile tons of stuff. So basically after using GeoInterface everything would have the "first time invocation overhead" again. That'd be really bad, esp. now that Julia 1.9 or 1.10 will add native code caching (i.e. generated machine code will be cached as part of precompilation -- speeding up TTFX quite a bit -- unless packages kill the benefit by invalidating methods...).

Noone suggested that you may misunderstand this PR.

If we invalidate anything with convert it would be with type pyracy, which is much higher up the list of "things not to do".

But we are all on board with using the package convert it's just what to do with the fact that we already have implementations of Base.convert in the wild.

fingolfin · 2022-12-31T15:25:09Z

src/fallbacks.jl

 end
 coordinates(t::AbstractFeatureCollectionTrait, fc) = [coordinates(f) for f in getfeature(fc)]

-Base.convert(T::Type, ::AbstractGeometryTrait, geom) = error("Conversion is enabled for type $T, but not implemented. Please report this issue to the package maintainer.")


This method is weird, but not actually a performance problem because the Julia compiler will never insert invocations to a three-argument Base.convert method

stop using Base.convert

f329ce5

visr requested a review from rafaqz August 3, 2022 22:29

move convert no-op to interface.jl

32df099

And expand the example a bit.

visr mentioned this pull request Nov 19, 2022

Differentiate between Base.convert and GeoInterface.convert JuliaGeo/LibGEOS.jl#146

Closed

fingolfin reviewed Dec 31, 2022

View reviewed changes

visr mentioned this pull request Jan 1, 2023

convert to module types #85

Merged

rafaqz merged commit f533e32 into main Jan 1, 2023

visr deleted the convert branch January 2, 2023 08:06

visr mentioned this pull request Jan 5, 2023

[breaking] switch from Base.convert to GeoInterface.convert yeesian/ArchGDAL.jl#359

Merged

stop using Base.convert #66

stop using Base.convert #66

Uh oh!

Conversation

visr commented Aug 3, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

visr commented Aug 3, 2022

Uh oh!

evetion commented Aug 7, 2022

Uh oh!

evetion commented Aug 7, 2022

Uh oh!

visr commented Aug 7, 2022

Uh oh!

visr commented Aug 7, 2022

Uh oh!

rafaqz commented Aug 8, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

evetion commented Aug 13, 2022

Uh oh!

jw3126 commented Nov 19, 2022

Uh oh!

rafaqz commented Nov 28, 2022

Uh oh!

evetion commented Nov 28, 2022

Uh oh!

rafaqz commented Nov 28, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

visr commented Dec 30, 2022

Uh oh!

fingolfin Dec 31, 2022

Choose a reason for hiding this comment

Uh oh!

rafaqz Dec 31, 2022

Choose a reason for hiding this comment

Uh oh!

fingolfin Dec 31, 2022

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

6 participants

visr commented Aug 3, 2022 •

edited

Loading

rafaqz commented Aug 8, 2022 •

edited

Loading

rafaqz commented Nov 28, 2022 •

edited

Loading